Add joined_default_parameter opt-in rule #1746

ornithocoder · 2017-08-03T09:54:05Z

to discourage explicit usage of the default separator. Implements #1093.

[x] Adds opt-in rule
[x] Adds to tests (+ Linux)
[x] Adds to changelog
[x] Updates Rules.md

SwiftLintBot · 2017-08-03T10:07:39Z

	8 Warnings
⚠️	Make sure that the docs are updated by running the `Generate docs` scheme.
⚠️	This PR introduced a violation in Nimble: /Sources/Nimble/FailureMessage.swift#L48:32: warning: Joined Default Parameter Violation: Discouraged explicit usage of the default separator. (joined_default_parameter)
⚠️	This PR introduced a violation in Sourcery: /Sourcery/Generating/Template/Swift/SwiftTemplate.swift#L61:53: warning: Joined Default Parameter Violation: Discouraged explicit usage of the default separator. (joined_default_parameter)
⚠️	This PR introduced a violation in Sourcery: /Sourcery/Generating/Template/Swift/SwiftTemplate.swift#L81:58: warning: Joined Default Parameter Violation: Discouraged explicit usage of the default separator. (joined_default_parameter)
⚠️	This PR introduced a violation in Sourcery: /Sourcery/Generating/Template/Swift/SwiftTemplate.swift#L257:29: warning: Joined Default Parameter Violation: Discouraged explicit usage of the default separator. (joined_default_parameter)
⚠️	This PR introduced a violation in Sourcery: /Sourcery/Parsing/FileParser.swift#L821:121: warning: Joined Default Parameter Violation: Discouraged explicit usage of the default separator. (joined_default_parameter)
⚠️	This PR introduced a violation in Sourcery: /SourceryTests/Helpers/Extensions.swift#L12:83: warning: Joined Default Parameter Violation: Discouraged explicit usage of the default separator. (joined_default_parameter)
⚠️	This PR introduced a violation in WordPress: /WordPress/Classes/Extensions/NSAttributedString+StyledHTML.swift#L65:54: warning: Joined Default Parameter Violation: Discouraged explicit usage of the default separator. (joined_default_parameter)

	12 Messages
📖	Linting Aerial with this PR took 0.36s vs 0.34s on master (5% slower)
📖	Linting Alamofire with this PR took 2.38s vs 2.33s on master (2% slower)
📖	Linting Firefox with this PR took 9.9s vs 9.86s on master (0% slower)
📖	Linting Kickstarter with this PR took 14.85s vs 14.72s on master (0% slower)
📖	Linting Moya with this PR took 1.01s vs 1.0s on master (1% slower)
📖	Linting Nimble with this PR took 1.37s vs 1.35s on master (1% slower)
📖	Linting Quick with this PR took 0.44s vs 0.44s on master (0% slower)
📖	Linting Realm with this PR took 2.08s vs 2.05s on master (1% slower)
📖	Linting SourceKitten with this PR took 0.78s vs 0.83s on master (6% faster)
📖	Linting Sourcery with this PR took 3.48s vs 3.55s on master (1% faster)
📖	Linting Swift with this PR took 9.95s vs 9.72s on master (2% slower)
📖	Linting WordPress with this PR took 9.42s vs 9.32s on master (1% slower)

Generated by 🚫 Danger

to discourage explicit usage of the default separator. Implements realm#1093.

codecov-io · 2017-08-03T11:56:27Z

Codecov Report

Merging #1746 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1746      +/-   ##
==========================================
+ Coverage   87.75%   87.78%   +0.02%     
==========================================
  Files         218      219       +1     
  Lines       10704    10730      +26     
==========================================
+ Hits         9393     9419      +26     
  Misses       1311     1311

Impacted Files	Coverage Δ
Tests/SwiftLintFrameworkTests/RulesTests.swift	`100% <100%> (ø)`	⬆️
...e/SwiftLintFramework/Rules/JoinedDefaultRule.swift	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54d6199...3252673. Read the comment docs.

ornithocoder · 2017-08-04T15:52:21Z

Should this one be opt-in or enabled by default @marcelofabri / @jpsim?

marcelofabri · 2017-08-05T11:24:16Z

I think it should be opt-in. By bet is that joined can be a common name.

marcelofabri · 2017-08-07T09:14:24Z

Source/SwiftLintFramework/Rules/JoinedDefaultRule.swift

+            let bodyOffset = dictionary.bodyOffset,
+            let bodyLength = dictionary.bodyLength else { return nil }
+
+        let body = file.contents.bridge().substringWithByteRange(start: bodyOffset, length: bodyLength)


what do you think about using the substructure instead?

{ "key.namelength": 10, "key.nameoffset": 1, "key.length": 25, "key.substructure": [{ "key.namelength": 9, "key.nameoffset": 12, "key.length": 13, "key.name": "separator", "key.bodyoffset": 23, "key.bodylength": 2, "key.kind": "source.lang.swift.expr.argument", "key.offset": 12 }], "key.name": "bar.joined", "key.bodyoffset": 12, "key.bodylength": 13, "key.kind": "source.lang.swift.expr.call", "key.offset": 1 }

It should be more reliable than the current implementation.

Sure. Consider it done :-)

marcelofabri · 2017-08-07T09:54:40Z

Source/SwiftLintFramework/Rules/JoinedDefaultRule.swift

-            let bodyOffset = dictionary.bodyOffset,
-            let bodyLength = dictionary.bodyLength else { return nil }
+            let argument = dictionary.substructure.first(where: { $0.name == "separator" }),
+            let argumentBodyOffset = argument.bodyOffset,


you can also check if argument.name == "separator"

I've just checked in another version which check the number of arguments and the argument name.

marcelofabri · 2017-08-07T10:03:14Z

Source/SwiftLintFramework/Rules/JoinedDefaultRule.swift

+    }
+
+    private func defaultSeparatorOffset(dictionary: [String: SourceKitRepresentable], file: File) -> Int? {
+        let arguments = dictionary.substructure.filter { $0.kind == SwiftExpressionKind.argument.rawValue }


you can use dictionary.enclosedArguments instead

marcelofabri · 2017-08-07T10:21:37Z

Rules.md

+
+### Examples
+
+<details>


It looks like the examples are outdated too.

marcelofabri · 2017-08-07T12:07:00Z

@ornithocoder what do you think about making it autocorrectable?

If you think it's easy, we can do on this PR. Otherwise we can leave it as a future improvement 👍

ornithocoder · 2017-08-07T12:38:37Z

@marcelofabri I'd do in a different PR, if that's OK. Thanks! :-)

marcelofabri · 2017-08-07T12:40:37Z

Sure, no problem. Thanks! 💯

as described in realm#1757 and realm#1746.

as described in #1757 and #1746.

Add joined_default_parameter opt-in rule

da823f5

to discourage explicit usage of the default separator. Implements realm#1093.

ornithocoder force-pushed the joined_default_parameter branch from 839497a to da823f5 Compare August 3, 2017 11:41

Move the violation mark to parameter offset

99c93f1

marcelofabri requested changes Aug 7, 2017

View reviewed changes

marcelofabri reviewed Aug 7, 2017

View reviewed changes

ornithocoder force-pushed the joined_default_parameter branch from 642c157 to be2c121 Compare August 7, 2017 09:59

marcelofabri reviewed Aug 7, 2017

View reviewed changes

Apply code review changes

3252673

ornithocoder force-pushed the joined_default_parameter branch from be2c121 to 3252673 Compare August 7, 2017 11:44

marcelofabri approved these changes Aug 7, 2017

View reviewed changes

marcelofabri merged commit 02f4212 into realm:master Aug 7, 2017

ornithocoder deleted the joined_default_parameter branch August 7, 2017 12:52

marcelofabri mentioned this pull request Aug 7, 2017

Rule Request: prefer 'joined()' over 'joined(separator: "")' #1093

Closed

ornithocoder mentioned this pull request Aug 8, 2017

Make joined_default_parameter rule correctable #1757

Closed

ornithocoder added a commit to ornithocoder/personal-fork-swiftlint that referenced this pull request Aug 8, 2017

Make joined_default_parameter rule correctable

72ca7a8

as described in realm#1757 and realm#1746.

ornithocoder mentioned this pull request Aug 8, 2017

Make joined_default_parameter rule correctable #1758

Merged

ornithocoder added a commit to ornithocoder/personal-fork-swiftlint that referenced this pull request Aug 8, 2017

Make joined_default_parameter rule correctable

bd1bc32

as described in realm#1757 and realm#1746.

ornithocoder added a commit to ornithocoder/personal-fork-swiftlint that referenced this pull request Aug 8, 2017

Make joined_default_parameter rule correctable

147a4d2

as described in realm#1757 and realm#1746.

marcelofabri pushed a commit that referenced this pull request Aug 17, 2017

Make joined_default_parameter rule correctable

b54e223

as described in #1757 and #1746.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add joined_default_parameter opt-in rule #1746

Add joined_default_parameter opt-in rule #1746

ornithocoder commented Aug 3, 2017 •

edited

Loading

SwiftLintBot commented Aug 3, 2017 •

edited

Loading

codecov-io commented Aug 3, 2017 •

edited

Loading

ornithocoder commented Aug 4, 2017 •

edited

Loading

marcelofabri commented Aug 5, 2017

marcelofabri Aug 7, 2017

ornithocoder Aug 7, 2017

marcelofabri Aug 7, 2017

ornithocoder Aug 7, 2017

marcelofabri Aug 7, 2017

ornithocoder Aug 7, 2017

marcelofabri Aug 7, 2017

ornithocoder Aug 7, 2017

marcelofabri commented Aug 7, 2017

ornithocoder commented Aug 7, 2017

marcelofabri commented Aug 7, 2017


		### Examples

		<details>

Add joined_default_parameter opt-in rule #1746

Add joined_default_parameter opt-in rule #1746

Conversation

ornithocoder commented Aug 3, 2017 • edited Loading

SwiftLintBot commented Aug 3, 2017 • edited Loading

codecov-io commented Aug 3, 2017 • edited Loading

Codecov Report

ornithocoder commented Aug 4, 2017 • edited Loading

marcelofabri commented Aug 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofabri commented Aug 7, 2017

ornithocoder commented Aug 7, 2017

marcelofabri commented Aug 7, 2017

ornithocoder commented Aug 3, 2017 •

edited

Loading

SwiftLintBot commented Aug 3, 2017 •

edited

Loading

codecov-io commented Aug 3, 2017 •

edited

Loading

ornithocoder commented Aug 4, 2017 •

edited

Loading